Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automate C code formatting [1/3] #395

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Feb 28, 2019

Let's introduce an automatic code style formatting and checks for Themis. We use clang-format and clang-tidy tools as they are generally available and gives reasonable results.

Introduce two new targets for Makefile:

  • make fmt
  • make fmt_check

fmt is intended for humans to reformat the code before submitting changes for review. Currently it formats only C code but it may be extended to other languages as well.

fmt_check is intended for CI to do format checks to ensure that the code is formatted accordingly. Unfortunately, clang-format does not have a convenient check mode so we hack one with git diff.

clang-tidy requires the compilation flags so we integrate the format checking into the build pipeline. It's a bit repetetive, but also gives up a chance to cleanup the build system. And it uses Make features nicely so we don't have to recheck and reformat the files which are okay.

We start by formatting only Soter and Themis source code. Other parts will be added later.

Supersedes #391, relates to #270

Let's introduce an automatic code style formatting and checks for
Themis. We use clang-format and clang-tidy tools as they are generally
available and gives reasonable results.

Introduce two new targets for Makefile:

  - make fmt
  - make fmt_check

"fmt" is intended for humans to reformat the code before submitting
changes for review. Currently it formats only C code but it may be
extended to other languages as well.

"fmt_check" is intended for CI to do format checks to ensure that the
code is formatted accordingly. Unfortunately, clang-format does not
have a convenient check mode so we hack one with git diff.

clang-tidy requires the compilation flags so we integrate the format
checking into the build pipeline. It's a bit repetetive, but also
gives up a chance to cleanup the build system. And it uses Make
features nicely so we don't have to recheck and reformat the files
which are okay.

We start by formatting only Soter and Themis source code. Other parts
will be added later.
@ilammy ilammy added core Themis Core written in C, its packages infrastructure Automated building and packaging labels Feb 28, 2019
@ilammy ilammy requested a review from ignatk February 28, 2019 12:39
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 28, 2019

@Lagovas voiced some concerns about readability and maintainability of these cryptic command lines, so here's a translation from Perl to Human:

$(CLANG_TIDY) -fix $< -- $(CFLAGS) 2>/dev/null \
    && $(CLANG_FORMAT) -i $< \
    && touch $@
	
$(CLANG_FORMAT) $< | diff -u $< - \
    && $(CLANG_TIDY) $< -- $(CFLAGS) 2>/dev/null \
    && touch $@

First we replace the variables:

  • CLANG_FORMAT — normally a clang-format binary, but may be overridden (like CC env variable)
  • CLANG_TIDY — ditto for the clang-tidy binary
  • CFLAGS — whatever compilation flags for C code we use
  • $< — the source file that needs to be formatted
    for example: src/themis/themis.h
  • $@ — marker file which tells Make that we have formatted the source
    for example: build/obj/themis/themis.h.fmt_fixup

Some options that we use:

  • clang-format -i formats the file in-place, without this option the formatted file is printed to stdout
  • clang-tidy -fix applies the suggested fixes in-place, without this option the file is left unchanged (but the suggestions are still printed to stdout)

We silence the stderr of clang-tidy because it likes to print silly statistics there which we don't need, but it looks like an error even if the file is okay.

This spell:

clang-format some/file | diff -u some/file -

compares the original file with output of clang-format (formatted file) that is printed to stdout. The - argument means "read from stdin". -u enables unified diff output.

Finally, we touch a marker file to tell Make that the source file has been successfully processed. With that the formatting will not be made again on subsequent calls to make.

I hope this helped you, lost soul who came to this PR searching for wisdom of the ancients after looking into git blame.

Copy link
Contributor

@shadinua shadinua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 28, 2019

thanks for explanation : )

@ignatk
Copy link
Contributor

ignatk commented Feb 28, 2019

  • let's drop [1/3] from the commit name (other commits would just say something like "apply make fmt")
  • you might want to add -q to the diff commands to not actually print the diff, just report the status
  • what is the point of marker files? In a typical workflow I can see devs doing some dev work (modifying the sources), then running make fmt (it will create the marker files), then the dev suddenly realises they made an error, so they modify the source again and run make fmt, but this time make fmt will not actually be executed, because the marker files are already there, which will confuse people - when they submit the PR, the CI will execute fmt_check on clean state, so may fail and the dev will be confused, because they ran make fmt before doing the PR.

In light of all this I would rather that these targets be ephemeral (without the marker files). Unless, you have some other use-case in mind

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 28, 2019

let's drop [1/3] from the commit name

Rest assured, I'll rename the resulting commits when merging these branches.

you might want to add -q to the diff commands to not actually print the diff

I left the diff output so that when the check fails on the CI then the build logs will immediately tell the dev what's wrong with the code style. The same goes for the developer manually checking the code with make fmt_check: it's quite useful to be able to see the diff, not only a status code which tells that something somewhere is wrong.

what is the point of marker files?

First of all, they allow us to hook those custom CMDs for the BUILD_CMD macro. We need to have separate targets for various source files as they have to be formatted differently.

Then they allow us to avoid reformatting the files which were already formatted. Keep in mind that make tracks the modification time of the files, and the marker files are created after the source files have been changed by the formatting tools. Thus we avoid reformatting the files that were not changed since they were formatted right.

If you first run make fmt, then change fileA.c, then run make fmt again — only fileA.c will be reformatted. Other files will not be processed if they did not change.

Copy link
Contributor

@ignatk ignatk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense

@ilammy ilammy merged commit 4d44b95 into cossacklabs:master Feb 28, 2019
@ilammy ilammy deleted the formatting-v2-1/3 branch March 28, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages infrastructure Automated building and packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants